Skip to content

Source NYT#259

Merged
Kilo59 merged 13 commits intoExpDev07:masterfrom
ibhuiyan17:source-nyt
Apr 13, 2020
Merged

Source NYT#259
Kilo59 merged 13 commits intoExpDev07:masterfrom
ibhuiyan17:source-nyt

Conversation

@ibhuiyan17
Copy link
Contributor

@ibhuiyan17 ibhuiyan17 commented Apr 4, 2020 •
edited
Loading

PR for adding the New York Times timeseries as a source, mentioned in #224

todo

  • Add unit tests for NYT source
  • Working v2 endpoints
  • Generate mock data
  • Add regression tests
  • Update documentation

** removed "Working v1 endpoints" from todo since current implementation of v1 doesn't support different sources

@ibhuiyan17 ibhuiyan17 changed the title [WIP] Source nyt [WIP] Source NYT Apr 4, 2020
@Kilo59 Kilo59 linked an issue Apr 7, 2020 that may be closed by this pull request
Using timeseries provided by NYT #224
Closed
@Kilo59 Kilo59 added enhancement New feature or request source: nyt New York Times labels Apr 7, 2020
Kilo59
Kilo59 requested changes Apr 8, 2020
View reviewed changes
Copy link
Collaborator

@Kilo59 Kilo59 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it's still WIP.
The ci failure is due to some pylint warnings and formatting issues.
The formatting can be applied by running invoke fmt

Here are the pylint warnings.

************* Module app.services.location.nyt
app/services/location/nyt.py:36:0: C0116: Missing function or method docstring (missing-function-docstring)
app/services/location/nyt.py:36:23: W0613: Unused argument 'category' (unused-argument)
app/services/location/nyt.py:48:4: W0107: Unnecessary pass statement (unnecessary-pass)
app/services/location/nyt.py:54:-1: W0105: String statement has no effect (pointless-string-statement)
app/services/location/nyt.py:2:0: W0611: Unused import csv (unused-import)
app/services/location/nyt.py:3:0: W0611: Unused datetime imported from datetime (unused-import)
app/services/location/nyt.py:8:0: W0611: Unused httputils imported from utils (unused-import)
-----------------------------------
Your code has been rated at 9.77/10

@ibhuiyan17
Copy link
Contributor Author

ibhuiyan17 commented Apr 8, 2020

Yep, still a work in progress, but thanks for pointing this out!

@ibhuiyan17 ibhuiyan17 marked this pull request as draft April 8, 2020 23:45
@nischalshankar
Copy link
Contributor

nischalshankar commented Apr 12, 2020

There were a few things we wanted to check in with you guys.

Output for NYT:
Currently, the NYT source doesn't provide coordinates, so we have just added null to those output values. NYT provides a fips number (specifies geographic location), which we could add to the output directly or potentially use it to translate to coordinates. Another option is we could use the (county, state) coordinates from csbs to find the coordinates of NYT locations, but this could be an inefficient solution. A way we could do this might be to store the specific coordinates in a file or a database.

Testing:
We tested all the v2 endpoints for NYT. The only content check we added is done for get locations which follows the same format as the other sources. Let us know if you want any additional content checks for any of the other sources. Also, we did not check for the specific value of country population since it could possibly change.

@ibhuiyan17 ibhuiyan17 marked this pull request as ready for review April 12, 2020 20:58
@ibhuiyan17 ibhuiyan17 requested a review from Kilo59 April 12, 2020 20:58
Kilo59
Kilo59 requested changes Apr 12, 2020
View reviewed changes
Copy link
Collaborator

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibhuiyan17 @nischalshankar Looks good!
I think nulling the coordinates values is a good approach for the initial implementation.
No reason to hold back using this source just because of that.

Just have a couple of minor points that I commented on below.

@Kilo59 Kilo59 requested a review from ExpDev07 April 12, 2020 22:13
@ibhuiyan17 ibhuiyan17 changed the title [WIP] Source NYT Source NYT Apr 13, 2020
@ibhuiyan17 ibhuiyan17 requested a review from Kilo59 April 13, 2020 00:31
@ibhuiyan17
Copy link
Contributor Author

ibhuiyan17 commented Apr 13, 2020

@Kilo59

Just finished resolving the points you commented on. Please let me know if there's anything else, thanks!

@Kilo59 Kilo59 merged commit 78da9fa into ExpDev07:master Apr 13, 2020
@Kilo59
Copy link
Collaborator

Kilo59 commented Apr 13, 2020

@all-contributors please add @nischalshankar for code, docs

@allcontributors allcontributors bot mentioned this pull request Apr 13, 2020
Merged
@allcontributors
Copy link
Contributor

allcontributors bot commented Apr 13, 2020

@Kilo59

I've put up a pull request to add @nischalshankar! 🎉

@Kilo59
Copy link
Collaborator

Kilo59 commented Apr 13, 2020

@all-contributors please add @ibhuiyan17 for docs

@allcontributors allcontributors bot mentioned this pull request Apr 13, 2020
Merged
@allcontributors
Copy link
Contributor

allcontributors bot commented Apr 13, 2020

@Kilo59

I've put up a pull request to add @ibhuiyan17! 🎉

@Kilo59 Kilo59 mentioned this pull request Apr 26, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kilo59 Kilo59 Kilo59 approved these changes

@ExpDev07 ExpDev07 Awaiting requested review from ExpDev07

Assignees

@ibhuiyan17 ibhuiyan17

Labels

enhancement New feature or request source: nyt New York Times

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using timeseries provided by NYT

3 participants

@ibhuiyan17 @nischalshankar @Kilo59